-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(drand): StateGetBeaconEntry
uses chain beacons for historical epochs
#12428
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
StateGetBeaconEntry
uses chain beacons for historical epochs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the PR title to match https://github.com/filecoin-project/lotus/blob/master/CONTRIBUTING.md#pr-title-conventions
b40852d
to
8c1d45f
Compare
I added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this should significantly reduce the quantity of nodes trying to query the old defunct incentinet network we lost 👍🏻
Opened #12452 to capture some thoughts about the 20 tipset walkback(s) in the code that I think are unnecessary. I just don't want to adjust the current get-beacon-from-chain logic in this PR so I left it alone. |
8c1d45f
to
b5c951a
Compare
Calling for objections before I land this. It would be great to get some more review eyes but I understand it's niche. |
b5c951a
to
92818c1
Compare
Per 2024-09-17 FilOz meeting, @jennijuju asked that @Stebalien or @Kubuxu take this to get this landed. @Stebalien @Kubuxu : see @rvagg 's offer for a sync discussion in https://filecoinproject.slack.com/archives/CP50PPW2X/p1726550218788619 if that is helpful Also, this one is needed for #12464 which we want to start this week. |
Notes from 2024-09-18 Lotus standup conversation: even though this has been opened for a couple of weeks, it affects pre-quicknet, and it was desired to get in as part of Lotus v1.29.2 this week, we won't merge it until it gets reviewed properly. We know @Stebalien and @Kubuxu are more tied up this week with a colo, but we assume (and we'll followup) that they'll engage next week. |
I will try to fetch in the context. I looked at it few weeks ago but I could not reach a conclusion at that time. |
92818c1
to
a49f214
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, apart from that one comment, where I feel iffy about the intent.
In the future, I suggest separating the commits into non-functional change refactors/renames and functional changes.
…pochs Fixes: #12414 Previously StateGetBeaconEntry would always try and use a drand beacon to get the appropriate round. But as drand has shut down old beacons and we've removed client details from Lotus, it has stopped working for historical beacons. This fix restores historical beacon entries by using the on-chain lookup, however it now follows the rules used by StateGetRandomnessFromBeacon and the get_beacon_randomness syscall which has some quirks with null rounds prior to nv14. See #12414 (comment) for specifics. StateGetBeaconEntry still blocks for future epochs and uses live drand beacon clients to wait for and fetch rounds as they are available.
a49f214
to
a7e15e3
Compare
…pochs (#12428) * fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs Fixes: #12414 Previously StateGetBeaconEntry would always try and use a drand beacon to get the appropriate round. But as drand has shut down old beacons and we've removed client details from Lotus, it has stopped working for historical beacons. This fix restores historical beacon entries by using the on-chain lookup, however it now follows the rules used by StateGetRandomnessFromBeacon and the get_beacon_randomness syscall which has some quirks with null rounds prior to nv14. See #12414 (comment) for specifics. StateGetBeaconEntry still blocks for future epochs and uses live drand beacon clients to wait for and fetch rounds as they are available. * fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs * fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs
…pochs (#12428) * fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs Fixes: #12414 Previously StateGetBeaconEntry would always try and use a drand beacon to get the appropriate round. But as drand has shut down old beacons and we've removed client details from Lotus, it has stopped working for historical beacons. This fix restores historical beacon entries by using the on-chain lookup, however it now follows the rules used by StateGetRandomnessFromBeacon and the get_beacon_randomness syscall which has some quirks with null rounds prior to nv14. See #12414 (comment) for specifics. StateGetBeaconEntry still blocks for future epochs and uses live drand beacon clients to wait for and fetch rounds as they are available. * fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs * fixup! fix(drand): `StateGetBeaconEntry` uses chain beacons for historical epochs
Fixes: #12414
Previously StateGetBeaconEntry would always try and use a drand beacon to get the appropriate round. But as drand has shut down old beacons and we've removed client details from Lotus, it has stopped working for historical beacons.
This fix restores historical beacon entries by using the on-chain lookup, however it now follows the rules used by StateGetRandomnessFromBeacon and the get_beacon_randomness syscall which has some quirks with null rounds prior to nv14. See #12414 (comment) for specifics.
StateGetBeaconEntry still blocks for future epochs and uses live drand beacon clients to wait for and fetch rounds as they are available.
Notes for review: the existing logic in
chain/rand/rand.go
should not change but I've had to do some refactoring to be able to return the beacon entries as well and not just the randomness. So the file looks a bit different. Please pay special attention to that because it's used for theget_beacon_randomness
syscall and theStateGetRandomnessFromBeacon
API. Aside from that, this should only impactStateGetBeaconEntry
.